Add targeted patch tool for Evernote notes #13
Merged
+437
−10
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Introduces the 'evernote_patch_note' tool for applying targeted find-and-replace operations to note content without regenerating the entire note. Implements backend logic in EvernoteAPI, updates types for replacements and patch results, and adds comprehensive integration tests for various patching scenarios.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds an Evernote "patch note" tool and API to perform ordered find-and-replace operations on note content, new types for replacements/results, updates to markdown↔ENML handling and resource preservation, and extensive integration tests (including a duplicated test block). Changes
Sequence DiagramsequenceDiagram
participant Client
participant MCPServer as MCP Server
participant EvernoteAPI
participant EvernoteService as Evernote Service
Client->>MCPServer: Call evernote_patch_note(guid, replacements)
MCPServer->>MCPServer: Validate input (replacements present, non-empty find)
MCPServer->>EvernoteAPI: patchNoteContent(guid, replacements)
EvernoteAPI->>EvernoteService: getNote(guid, includeResources)
EvernoteService-->>EvernoteAPI: Note with ENML & resources
EvernoteAPI->>EvernoteAPI: Convert ENML to Markdown
EvernoteAPI->>EvernoteAPI: Apply replacements sequentially (track occurrences/replaced)
EvernoteAPI->>EvernoteAPI: Validate result (changes occurred, not empty)
EvernoteAPI->>EvernoteAPI: Convert Markdown back to ENML (preserve resources)
EvernoteAPI->>EvernoteService: updateNote(updatedNote)
EvernoteService-->>EvernoteAPI: Update confirmation
EvernoteAPI-->>MCPServer: PatchNoteResult(success, changes[], optional warning)
MCPServer-->>Client: Human-readable success/failure summary with details
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
__tests__/integration/mcp-tools.test.ts (1)
82-82: Test assertion is outdated and will fail with the actual tool count.The
expect(result.tools).toHaveLength(11)assertion doesn't match the actual tools registered. ThelistToolsHandlerinsrc/index.tsreturns the completetoolsarray containing all 26 tools (including the newevernote_patch_note), not just 11. Update the test to:
- Change the expected length from 11 to 26
- Add
evernote_patch_noteto the tools being checked
🤖 Fix all issues with AI agents
In `@src/evernote-api.ts`:
- Around line 134-170: patchNoteContent currently calls applyMarkdownToNote
after replacements which causes unreferenced resources to be dropped; update
patchNoteContent to preserve existing note.resources when applying the patched
markdown (or modify applyMarkdownToNote to merge resources) so attachments not
referenced in the new markdown are retained. Specifically, in patchNoteContent
(after getNote and convertENMLToMarkdown) ensure you pass the original
note.resources (or merge them inside applyMarkdownToNote) instead of letting
applyMarkdownToNote unconditionally replace resources; mirror the behavior used
by createNote which preserves and adds to existing resources. Make sure getNote,
convertENMLToMarkdown, and applyMarkdownToNote cooperate to avoid deleting
attachments that weren't explicitly removed by the replacement.
🧹 Nitpick comments (1)
src/index.ts (1)
1389-1422: Consider validating emptyfindstrings.The handler validates that
replacementsis a non-empty array, but doesn't check for emptyfindstrings. An emptyfindstring would match at every position in the content, leading to unexpected behavior:"abc".split("").join("X") // Returns "XaXbXcX"🔧 Suggested validation enhancement
case 'evernote_patch_note': { const { guid, replacements } = args as any; if (!replacements || !Array.isArray(replacements) || replacements.length === 0) { throw new Error('At least one replacement must be provided'); } + for (const r of replacements) { + if (!r.find || typeof r.find !== 'string' || r.find.length === 0) { + throw new Error('Each replacement must have a non-empty "find" string'); + } + } const result = await evernoteApi.patchNoteContent(guid, replacements);
- Preserve existing resources during patch operation by saving original resources and merging them back after markdown conversion - Add validation for empty find strings to prevent unexpected behavior - Update test assertion for tool count from 11 to 28 - Add evernote_patch_note to tested tool names - Add tests for empty find string validation and resource preservation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…Note
Instead of post-hoc restoration after calling applyMarkdownToNote,
add a preserveResources option that handles the merge internally.
This is cleaner and follows the single-responsibility principle.
- Add optional { preserveResources: boolean } parameter to applyMarkdownToNote
- When enabled, merges original resources with new attachments
- patchNoteContent now simply passes { preserveResources: true }
- Maintains backward compatibility (default behavior unchanged)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Move & decoding to last position when unescaping HTML entities. This prevents security issues where &lt; would incorrectly become < instead of < Fixes CodeQL alert #7 (js/double-escaping) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Member
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/evernote-api.ts`:
- Around line 198-257: The patchNoteContent flow should validate inputs before
performing I/O: check that the replacements array (parameter replacements:
NoteReplacement[]) is non-empty and that each replacement has a non-empty find
string (and optionally a replace value) before calling getNote(guid, true,
true); if validation fails return the same shape PatchNoteResult with
success:false, noteGuid:guid, an empty or appropriate changes array and a
warning message (e.g., "No replacements provided" or "Empty find string in
replacements") instead of fetching the note; implement this logic at the start
of patchNoteContent to prevent runaway/expensive operations and reference the
function name patchNoteContent and the types NoteReplacement and PatchNoteResult
when making the change.
Comment on lines
198
to
257
| async patchNoteContent(guid: string, replacements: NoteReplacement[]): Promise<PatchNoteResult> { | ||
| // Fetch existing note with content and resources | ||
| const note = await this.getNote(guid, true, true); | ||
|
|
||
| // Convert ENML to markdown | ||
| let markdown = this.convertENMLToMarkdown(note.content, note.resources); | ||
|
|
||
| // Track changes | ||
| const changes: PatchNoteResult['changes'] = []; | ||
|
|
||
| // Apply replacements sequentially | ||
| for (const replacement of replacements) { | ||
| const { find, replace, replaceAll = true } = replacement; | ||
|
|
||
| // Count occurrences | ||
| const regex = new RegExp(find.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'), 'g'); | ||
| const matches = markdown.match(regex); | ||
| const occurrences = matches ? matches.length : 0; | ||
|
|
||
| // Perform replacement | ||
| let replaced = 0; | ||
| if (occurrences > 0) { | ||
| if (replaceAll) { | ||
| markdown = markdown.split(find).join(replace); | ||
| replaced = occurrences; | ||
| } else { | ||
| markdown = markdown.replace(find, replace); | ||
| replaced = 1; | ||
| } | ||
| } | ||
|
|
||
| changes.push({ | ||
| find, | ||
| occurrences, | ||
| replaced, | ||
| }); | ||
| } | ||
|
|
||
| // Check if any changes were made | ||
| const totalReplaced = changes.reduce((sum, c) => sum + c.replaced, 0); | ||
| if (totalReplaced === 0) { | ||
| return { | ||
| success: false, | ||
| noteGuid: guid, | ||
| changes, | ||
| warning: 'No matches found for any replacement patterns', | ||
| }; | ||
| } | ||
|
|
||
| // Check if content would be empty after replacement | ||
| const trimmedMarkdown = markdown.trim(); | ||
| if (!trimmedMarkdown) { | ||
| return { | ||
| success: false, | ||
| noteGuid: guid, | ||
| changes, | ||
| warning: 'Replacement would result in empty note content - operation aborted', | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the NoteReplacement type definition
rg "interface NoteReplacement|type NoteReplacement" -A 5 src/Repository: verygoodplugins/mcp-evernote
Length of output: 246
🏁 Script executed:
# Check the full patchNoteContent implementation for context
cat -n src/evernote-api.ts | sed -n '198,260p'Repository: verygoodplugins/mcp-evernote
Length of output: 2446
🏁 Script executed:
# Look for any existing validation in the file
rg "throw new Error|if.*replacements" src/evernote-api.ts -B 2 -A 2Repository: verygoodplugins/mcp-evernote
Length of output: 54
🏁 Script executed:
# Check if there are type guards or runtime checks already
rg "PatchNoteResult|NoteReplacement" src/ -B 1 -A 3Repository: verygoodplugins/mcp-evernote
Length of output: 1250
Add defensive validation for empty replacements and find strings before I/O.
An empty find string creates a regex matching every character boundary, causing runaway replacements and performance issues. An empty replacements array still triggers an unnecessary note fetch. Validate both inputs before calling getNote().
Suggested fix
async patchNoteContent(guid: string, replacements: NoteReplacement[]): Promise<PatchNoteResult> {
+ if (!Array.isArray(replacements) || replacements.length === 0) {
+ throw new Error('At least one replacement must be provided');
+ }
+ for (const { find } of replacements) {
+ if (!find) {
+ throw new Error('Each replacement must have a non-empty "find" string');
+ }
+ }
// Fetch existing note with content and resources
const note = await this.getNote(guid, true, true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async patchNoteContent(guid: string, replacements: NoteReplacement[]): Promise<PatchNoteResult> { | |
| // Fetch existing note with content and resources | |
| const note = await this.getNote(guid, true, true); | |
| // Convert ENML to markdown | |
| let markdown = this.convertENMLToMarkdown(note.content, note.resources); | |
| // Track changes | |
| const changes: PatchNoteResult['changes'] = []; | |
| // Apply replacements sequentially | |
| for (const replacement of replacements) { | |
| const { find, replace, replaceAll = true } = replacement; | |
| // Count occurrences | |
| const regex = new RegExp(find.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'), 'g'); | |
| const matches = markdown.match(regex); | |
| const occurrences = matches ? matches.length : 0; | |
| // Perform replacement | |
| let replaced = 0; | |
| if (occurrences > 0) { | |
| if (replaceAll) { | |
| markdown = markdown.split(find).join(replace); | |
| replaced = occurrences; | |
| } else { | |
| markdown = markdown.replace(find, replace); | |
| replaced = 1; | |
| } | |
| } | |
| changes.push({ | |
| find, | |
| occurrences, | |
| replaced, | |
| }); | |
| } | |
| // Check if any changes were made | |
| const totalReplaced = changes.reduce((sum, c) => sum + c.replaced, 0); | |
| if (totalReplaced === 0) { | |
| return { | |
| success: false, | |
| noteGuid: guid, | |
| changes, | |
| warning: 'No matches found for any replacement patterns', | |
| }; | |
| } | |
| // Check if content would be empty after replacement | |
| const trimmedMarkdown = markdown.trim(); | |
| if (!trimmedMarkdown) { | |
| return { | |
| success: false, | |
| noteGuid: guid, | |
| changes, | |
| warning: 'Replacement would result in empty note content - operation aborted', | |
| }; | |
| } | |
| async patchNoteContent(guid: string, replacements: NoteReplacement[]): Promise<PatchNoteResult> { | |
| if (!Array.isArray(replacements) || replacements.length === 0) { | |
| throw new Error('At least one replacement must be provided'); | |
| } | |
| for (const { find } of replacements) { | |
| if (!find) { | |
| throw new Error('Each replacement must have a non-empty "find" string'); | |
| } | |
| } | |
| // Fetch existing note with content and resources | |
| const note = await this.getNote(guid, true, true); | |
| // Convert ENML to markdown | |
| let markdown = this.convertENMLToMarkdown(note.content, note.resources); | |
| // Track changes | |
| const changes: PatchNoteResult['changes'] = []; | |
| // Apply replacements sequentially | |
| for (const replacement of replacements) { | |
| const { find, replace, replaceAll = true } = replacement; | |
| // Count occurrences | |
| const regex = new RegExp(find.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'), 'g'); | |
| const matches = markdown.match(regex); | |
| const occurrences = matches ? matches.length : 0; | |
| // Perform replacement | |
| let replaced = 0; | |
| if (occurrences > 0) { | |
| if (replaceAll) { | |
| markdown = markdown.split(find).join(replace); | |
| replaced = occurrences; | |
| } else { | |
| markdown = markdown.replace(find, replace); | |
| replaced = 1; | |
| } | |
| } | |
| changes.push({ | |
| find, | |
| occurrences, | |
| replaced, | |
| }); | |
| } | |
| // Check if any changes were made | |
| const totalReplaced = changes.reduce((sum, c) => sum + c.replaced, 0); | |
| if (totalReplaced === 0) { | |
| return { | |
| success: false, | |
| noteGuid: guid, | |
| changes, | |
| warning: 'No matches found for any replacement patterns', | |
| }; | |
| } | |
| // Check if content would be empty after replacement | |
| const trimmedMarkdown = markdown.trim(); | |
| if (!trimmedMarkdown) { | |
| return { | |
| success: false, | |
| noteGuid: guid, | |
| changes, | |
| warning: 'Replacement would result in empty note content - operation aborted', | |
| }; | |
| } |
🤖 Prompt for AI Agents
In `@src/evernote-api.ts` around lines 198 - 257, The patchNoteContent flow should
validate inputs before performing I/O: check that the replacements array
(parameter replacements: NoteReplacement[]) is non-empty and that each
replacement has a non-empty find string (and optionally a replace value) before
calling getNote(guid, true, true); if validation fails return the same shape
PatchNoteResult with success:false, noteGuid:guid, an empty or appropriate
changes array and a warning message (e.g., "No replacements provided" or "Empty
find string in replacements") instead of fetching the note; implement this logic
at the start of patchNoteContent to prevent runaway/expensive operations and
reference the function name patchNoteContent and the types NoteReplacement and
PatchNoteResult when making the change.
Add early validation in patchNoteContent to check replacements array before calling getNote. Returns PatchNoteResult with success:false and appropriate warning instead of performing expensive I/O operations. Validates: - replacements array is non-empty - each NoteReplacement has a non-empty find string Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduces the 'evernote_patch_note' tool for applying targeted find-and-replace operations to note content without regenerating the entire note. Implements backend logic in EvernoteAPI, updates types for replacements and patch results, and adds comprehensive integration tests for various patching scenarios.